feat(sandbox): add Firecracker disk I/O rate limiting (bandwidth + IOPS) by rogeroger-yu · Pull Request #48 · kvcache-ai/AgentENV

rogeroger-yu · 2026-07-28T14:03:39Z

Summary

Wire Firecracker's per-drive TokenBucket rate limiter into the sandbox lifecycle
Configurable via [machine.disk_rate_limit] with bandwidth, IOPS, and one-time burst settings (refill period is fixed at 1s so *_per_sec values are the true sustained rate)
Applied at both fresh boot (pre-boot Drive::rate_limiter) and snapshot resume (PATCH /drives, reconciled before resume())

Closes #46

Notes on burst semantics

bandwidth_burst_bytes / iops_burst map to Firecracker's one_time_burst: a separate allowance granted once at VM start, consumed before the sustained bucket and not replenished. It absorbs the initial boot I/O spike; it does not raise the steady-state rate.

Changes

config/default.toml: Add [machine.disk_rate_limit] section (disabled by default)
src/cfg.rs: Add DiskRateLimitConfig with validation (disabled -> skip; reject burst-without-sustained; reject values > i64::MAX)
src/sandbox/firecracker/config.rs: Carry effective config in FirecrackerCommonConfig so fresh boot and resume apply the same config and it persists across pause/resume
src/sandbox/firecracker/instance.rs: Add patch_drive_rate_limiter() for the resume path
src/sandbox/firecracker/sandbox.rs: Build the pre-boot limiter for fresh drives; on resume, reconcile per dimension and use an all-zero token bucket to explicitly disable an inherited dimension

Test plan

Build passes (cargo build --release)
Baseline: sandbox disk I/O without limiting (~1.8 GB/s write)
Rate-limited: 100 MB/s + 3000 IOPS configured (~12-28 MB/s)
Resume path: limiter reconciled before resume(); inherited limiter cleared when disabled
Integration test with cargo test

github-actions · 2026-07-28T14:31:05Z

🔍 OpenCodeReview found 3 issue(s) in this PR.

✅ Successfully posted inline: 3 comment(s)

github-actions · 2026-07-28T14:31:06Z

+# bandwidth_bytes_per_sec = 104857600   # 100 MB/s
+# bandwidth_burst_bytes = 10485760      # 10 MB burst
+# iops = 3000
+# iops_burst = 500
+# refill_time_ms = 1000


The *_per_sec naming/documentation is inconsistent with how this configuration is converted to Firecracker token buckets: these values are passed directly as the bucket size, while refill_time_ms controls how often that many tokens are replenished. Thus bandwidth_bytes_per_sec = 104857600 is 100 MB/s only at 1000 ms; setting 500 ms yields 200 MB/s, and iops behaves similarly. Either convert per-second rates into bucket sizes based on refill_time_ms, or rename these options to bucket-size values and document the resulting rate semantics.

github-actions · 2026-07-28T14:31:06Z

+    /// Sustained disk bandwidth limit in bytes per second (0 = unlimited).
+    #[config(default = 0u64)]
+    pub bandwidth_bytes_per_sec: u64,


This value is described as a per-second rate, but the Firecracker integration passes it directly as the token-bucket size. The effective rate is size / refill_time, so any configured refill_time_ms other than 1000 changes the requested bytes/second (for example, 500 ms doubles it). Either remove/configure a fixed 1000 ms refill period, or convert the per-second value to a bucket size using checked arithmetic before constructing the limiter.

github-actions · 2026-07-28T14:31:06Z

+    /// Token bucket refill period in milliseconds.
+    #[config(default = 1000u64)]
+    pub refill_time_ms: u64,


These public values are later converted from u64 to Firecracker's i64 fields with as, but there is no configuration validation. Values above i64::MAX wrap to negative numbers, and refill_time_ms = 0 creates an invalid token bucket whenever bandwidth or IOPS limiting is configured, causing sandbox startup/restore to fail at the Firecracker API boundary. Validate enabled limiter values (refill_time_ms > 0 and every converted value <= i64::MAX) during AppConfig::validate, or use checked conversions when building the limiter.

github-actions · 2026-07-28T14:31:06Z

+        let mut bw = firecracker_client::models::TokenBucket::new(
+            cfg.refill_time_ms as i64,
+            cfg.bandwidth_bytes_per_sec as i64,
+        );


TokenBucket::size is the number of tokens added per refill_time, not a per-second rate. Passing bandwidth_bytes_per_sec directly only produces the configured rate when refill_time_ms == 1000; for example, 100 MB/s with 500 ms refills becomes 200 MB/s. The IOPS bucket below has the same issue. Scale each configured per-second rate by refill_time_ms / 1000 (with checked arithmetic and a documented rounding policy), or remove the configurable refill period.

github-actions · 2026-07-28T14:31:07Z

+            cfg.refill_time_ms as i64,
+            cfg.bandwidth_bytes_per_sec as i64,


These values are user-configurable u64s, and as i64 silently wraps values above i64::MAX into negative API fields. The burst and IOPS conversions have the same problem. Validate all fields with i64::try_from and propagate a configuration error; this requires making build_disk_rate_limiter return Result<Option<_>>.

Wire Firecracker's per-drive TokenBucket rate limiter into the sandbox lifecycle. Configurable via [machine.disk_rate_limit] with bandwidth (bytes/sec), IOPS caps, burst allowances, and refill interval. Applied at both fresh-boot (add_drive) and snapshot-resume (PATCH /drives) paths so all sandboxes are governed regardless of launch mode.

github-actions · 2026-07-29T02:43:44Z

+# bandwidth_bytes_per_sec = 104857600   # 100 MB/s
+# bandwidth_burst_bytes = 10485760      # 10 MB burst
+# iops = 3000
+# iops_burst = 500
+# refill_time_ms = 1000


These names/comments promise per-second limits, but the implementation passes bandwidth_bytes_per_sec and iops directly as Firecracker token-bucket sizes. The effective rate is bucket size per refill_time_ms, so changing refill_time_ms from 1000 changes the configured per-second rate (for example, 500 ms doubles it). Either remove/configure a fixed 1000 ms refill interval, rename the fields as per-refill bucket sizes, or scale the bucket sizes by refill_time_ms / 1000 with overflow/range validation.

github-actions · 2026-07-29T02:43:44Z

+        let mut bw = firecracker_client::models::TokenBucket::new(
+            cfg.refill_time_ms as i64,
+            cfg.bandwidth_bytes_per_sec as i64,
+        );


TokenBucket::size is the number of tokens replenished per refill_time, but this passes a per-second value unchanged. Therefore any configured refill period other than 1000 ms changes the actual sustained rate (for example, 100 MB/s with 500 ms becomes 200 MB/s). Scale the bandwidth and IOPS bucket sizes by refill_time_ms / 1000, or remove the configurable refill period. Please also use checked arithmetic/conversions because these u64 as i64 casts (and the equivalent IOPS/burst casts below) wrap values above i64::MAX into invalid negative Firecracker fields instead of returning a configuration error.

Suggestion:

Suggested change

let mut bw = firecracker_client::models::TokenBucket::new(

cfg.refill_time_ms as i64,

cfg.bandwidth_bytes_per_sec as i64,

);

let refill_time = i64::try_from(cfg.refill_time_ms)

.context("disk rate-limit refill_time_ms exceeds Firecracker's i64 range")?;

let bandwidth_size = cfg

.bandwidth_bytes_per_sec

.checked_mul(cfg.refill_time_ms)

.and_then(|value| value.checked_div(1000))

.and_then(|value| i64::try_from(value).ok())

.context("disk bandwidth rate limit is outside Firecracker's supported range")?;

let mut bw = firecracker_client::models::TokenBucket::new(refill_time, bandwidth_size);

github-actions · 2026-07-29T02:43:44Z

+        if let Some(rl) = build_disk_rate_limiter(disk_rl_cfg) {
+            self.fc_instance
+                .patch_drive_rate_limiter(USER_ROOTFS_DRIVE_ID, rl)
+                .await
+                .context("apply disk rate limiter after resume")?;
+        }


When rate limiting is disabled (or both limits are zero), this skips the PATCH entirely. A restored snapshot can already contain the user drive's previous rate limiter, so disabling the current configuration will leave that snapshotted limiter active. The resume path should always reconcile the drive with current configuration, including explicitly patching an empty/default RateLimiter to remove an inherited limiter (assuming Firecracker's empty limiter representation), rather than treating None as “do nothing.”

huang-jl · 2026-07-29T02:57:04Z

+# bandwidth_burst_bytes = 10485760      # 10 MB burst
+# iops = 3000
+# iops_burst = 500
+# refill_time_ms = 1000


As pointed out in the review, the current configuration exposes too much complexity to users. I'm considering dropping refill_time_ms and hardcoding it to 1000ms instead.

Address review feedback on the Firecracker disk I/O rate limiter: - Drop the configurable refill_time_ms and pin the token-bucket refill period to 1000 ms, so the configured *_per_sec values are the actual sustained per-second rates. - Replace lossy `as i64` casts with checked conversions that surface an error instead of silently truncating oversized byte/IOPS values. - Unify the fresh-boot and resume paths behind a single post-boot PATCH helper, and remove the now-unused rate_limiter argument from add_drive. - Fix resume reconciliation: a restored snapshot inherits its previous limiter, and an empty RateLimiter PATCH is a no-op because Firecracker treats an absent bucket as "leave unchanged". When the current config disables limiting, overwrite both buckets with an effectively-unlimited bucket so the inherited throttle is actually cleared. Add unit tests covering bucket construction, the disabled/zero cases, checked-conversion overflow, and the unlimited-limiter override.

rogeroger-yu · 2026-08-04T06:38:07Z

Follow-up: fixed resume reconciliation + e2e validation

Pushed ab2ab1f addressing the review feedback (pinned refill to 1000 ms, checked i64 conversions, unified fresh-boot/resume PATCH path, removed the unused add_drive rate_limiter arg). While running the full e2e — including the resume path — I found the original resume-clear logic was silently ineffective, so this commit also fixes it. Details below.

e2e setup

Cold-start sandbox from a public image (ghcr.io/linuxserver/baseimage-ubuntu:noble), writing to the rate-limited user rootfs drive (vdb, mounted at /) with dd ... oflag=direct. Limiter config: bandwidth_bytes_per_sec = 104857600 (100 MiB/s), burst 10 MiB, iops = 3000. A 2 GB transfer is used for the steady-state number (a 512 MB transfer is skewed by the one-time burst and doesn't reach steady state).

Results

Scenario	2 GB `oflag=direct` write	Interpretation
Fresh boot, limiter enabled	110 MB/s (≈104.9 MiB/s)	throttled to the configured rate ✅
Fresh boot, limiter disabled (control)	1.4 GB/s	native storage ceiling
Resume under disabled config, old empty-PATCH clear	110 MB/s	throttle persisted — bug ❌
Resume under disabled config, fixed clear	1.3 GB/s	inherited limiter cleared ✅

The control run (fresh + disabled = 1.4 GB/s) rules out "110 MB/s is just the disk ceiling": the disk can clearly do >10x that, so a resumed sandbox stuck at 110 MB/s is still being throttled.

Root cause

A Firecracker snapshot persists the block device's rate-limiter state, so a resumed VM inherits whatever limiter was active when it was paused. The original code tried to clear this by PATCHing an empty RateLimiter. That is a no-op: Firecracker's PATCH /drives maps an absent token bucket to BucketUpdate::None ("leave unchanged"), so an empty limiter changes nothing and the inherited throttle survives.

I confirmed this directly against the Firecracker API socket of a resumed sandbox:

PATCH /drives/user_rootfs  {"rate_limiter":{}}                       -> HTTP 204, still 110 MB/s
PATCH /drives/user_rootfs  {"rate_limiter":{bandwidth:{size:1e12,refill_time:1}, ops:{...}}}
                                                                     -> HTTP 204, 1.1 GB/s

Fix

Since Firecracker offers no way to remove a limiter via PATCH, when the current node config disables limiting on resume we overwrite both buckets with an effectively-unlimited bucket (size = 1 TiB, refill = 1 ms). That dwarfs any real disk, so throttling no longer bites. Fresh boots are unaffected (their device model starts clean and skip the PATCH). Added a unit test for the override plus the checked-conversion cases.

github-actions · 2026-08-04T07:52:36Z

+    #[config(default = 0u64)]
+    pub bandwidth_bytes_per_sec: u64,
+    /// One-time burst allowance in bytes above the sustained bandwidth.
+    #[config(default = 0u64)]
+    pub bandwidth_burst_bytes: u64,
+    /// Sustained IOPS limit (0 = unlimited).
+    #[config(default = 0u64)]
+    pub iops: u64,
+    /// One-time burst allowance in operations above the sustained IOPS.
+    #[config(default = 0u64)]
+    pub iops_burst: u64,


[bug · medium]
These u64 fields admit values above Firecracker's signed i64 token-bucket range. Such a configuration passes AppConfig::validate() and only fails later whenever a sandbox starts, even though it is statically invalid. Validate all four values against i64::MAX during config loading (and preferably reject a nonzero burst when its corresponding sustained limit is zero) so startup reports the bad configuration immediately.

github-actions · 2026-08-04T07:52:36Z

                .or_else(|| Some(DEFAULT_BOOT_ARGS.to_string())),
            vcpu_count: config.machine.vcpu_count,
            mem_size_mib: config.machine.mem_size_mib,
+            disk_rate_limit: config.machine.disk_rate_limit.clone(),


[bug · medium]
This copied per-sandbox setting is never consumed: the launch path applies ConfigManager::global_config().machine.disk_rate_limit instead of FirecrackerSandboxConfig::disk_rate_limit. Consequently, callers of from_app_config_with_user_image using a non-global AppConfig can configure one limit here but launch with another. Pass this field into the rate-limiter application path (or remove it and explicitly require global configuration) so the constructor's configuration is honored.

github-actions · 2026-08-04T07:52:36Z

        self.fc_instance.start().await?;
+        self.apply_disk_rate_limiter(false).await?;


[security · high]
Install the limiter before starting guest execution. As written, the guest can issue unrestricted disk I/O between start() and this PATCH; if the PATCH fails, startup returns an error while Firecracker remains running without the configured isolation. Fresh drives already support Drive::rate_limiter, so pass the limiter when configuring the user-rootfs drive (or explicitly stop the VM on PATCH failure).

I think the correct way to apply the disk rate limiter for start_fresh is to configure it during configure_microvm -> add_drive(USER_ROOTFS_DRIVE_ID, ...), rather than patching it after the microVM has started.

That said, this would likely require more extensive code changes.

Our development team needs to discuss this internally before finalizing our review, I will follow up soon.

github-actions · 2026-08-04T07:52:36Z

        self.fc_instance.resume().await?;

+        // A restored snapshot may carry a previously configured limiter, so
+        // reconcile against the node's current config, clearing any inherited
+        // limiter when disk rate limiting is now disabled.
+        self.apply_disk_rate_limiter(true).await?;


[security · high]
Reconcile the restored drive while the VM is still paused, before calling resume(). This ordering allows a snapshot's stale/disabled limiter—or no limiter—to govern disk I/O immediately after resume, and a failed PATCH leaves a running VM even though restore is reported as failed. PATCH the loaded paused device first; if Firecracker cannot accept that state transition, add rollback that stops the VM when reconciliation fails.

This AI review is correct. Consider moving apply_disk_rate_limiter to after load_snapshot_file but before resume().

github-actions · 2026-08-04T07:52:37Z

+        let cfg = &ConfigManager::global_config().machine.disk_rate_limit;
+        let rl = match build_disk_rate_limiter(cfg)? {


[bug · medium]
This ignores FirecrackerSandboxConfig::disk_rate_limit, so a fresh sandbox created from an explicit AppConfig can be throttled using a different global setting. The newly captured launch-config field is otherwise unused. Pass the fresh launch configuration into this method (while the snapshot-resume path can intentionally use the current global configuration), so the configuration used to construct the sandbox is the one applied.

This is also correct. The disk_rate_limit field you added to FirecrackerSandboxConfig is currently unused, it's never read in apply_disk_rate_limiter.

There are two ways to resolve this:

Drop the new field entirely and have apply_disk_rate_limiter read from the global configuration.

Actually read FirecrackerSandboxConfig.disk_rate_limit in apply_disk_rate_limiter, so it's actually consumed.

I'd prefer the second option.

apply_disk_rate_limiter previously always read the global config, leaving the disk_rate_limit captured on FirecrackerSandboxConfig unused. A fresh sandbox built from an explicit AppConfig could thus be throttled with a different global setting than the one used to construct it. Pass the config source in explicitly: fresh boot uses the sandbox's own launch config, while snapshot resume intentionally reconciles against the node's current global config.

github-actions · 2026-08-05T08:04:03Z

+    #[config(default = 0u64)]
+    pub bandwidth_burst_bytes: u64,
+    /// Sustained IOPS limit (0 = unlimited).
+    #[config(default = 0u64)]
+    pub iops: u64,
+    /// One-time burst allowance in operations above the sustained IOPS.
+    #[config(default = 0u64)]
+    pub iops_burst: u64,


[bug · medium]
Validate that each burst is zero unless its corresponding sustained limit is nonzero. The application code only creates a bandwidth/IOPS token bucket when bandwidth_bytes_per_sec/iops is greater than zero, so configurations such as bandwidth_bytes_per_sec = 0 with bandwidth_burst_bytes = 1024 are accepted but silently ignore the burst. Reject these inconsistent combinations during AppConfig::validate() so operator mistakes fail at configuration load time.

github-actions · 2026-08-05T08:04:03Z

+        self.apply_disk_rate_limiter(&config.disk_rate_limit, false)
+            .await?;


[bug · high]
The guest is already running when this fallible PATCH executes. It can issue unthrottled I/O before the request completes, and if conversion or PATCH fails, start() returns Err while this &mut self sandbox can still own a running microVM. Apply the limiter as part of the pre-boot drive configuration (the Drive model supports rate_limiter), or otherwise ensure the VM cannot execute until the PATCH succeeds and explicitly stop it on failure. The restore path has the same ordering problem because it resumes before reconciling the limiter.

github-actions · 2026-08-05T08:04:03Z

+        let rl = match build_disk_rate_limiter(cfg)? {
+            Some(rl) => rl,
+            None if clear_inherited => unlimited_rate_limiter(),
+            None => return Ok(()),
+        };


[bug · medium]
Snapshot reconciliation only clears inherited buckets when both configured dimensions are absent. If a snapshot has bandwidth and IOPS limits and the current config enables bandwidth only, build_disk_rate_limiter omits ops; by the PATCH semantics documented above, that means “leave unchanged,” so the old IOPS limit remains active (and vice versa). On the resume/reconciliation path, explicitly overwrite each zero-valued dimension with an unlimited bucket while retaining the configured bucket for the other dimension.

…mension Address review feedback on the disk rate limiter: - cfg: reject a burst configured without a nonzero sustained rate (bandwidth_burst_bytes/iops_burst require bandwidth_bytes_per_sec/iops), which would otherwise be silently ineffective. - fresh boot: attach the limiter as pre-boot drive config (PUT /drives) instead of a post-start PATCH, closing the window where the guest could issue unthrottled I/O before the limiter was applied. Re-adds the rate_limiter parameter to Instance::add_drive. - resume: move the reconcile PATCH to before resume() (while the VM is loaded-but-paused) so the limiter is in force the instant the guest runs. - resume reconcile is now per-dimension: both buckets are always sent (configured bucket if set, otherwise an effectively-unlimited bucket). Firecracker treats an omitted bucket in PATCH as "leave unchanged", so an inherited dimension the current config leaves unset must be overwritten rather than omitted, or a snapshot-inherited limit would survive resume.

huang-jl · 2026-08-05T14:02:55Z

+        // inherited dimension the current config leaves unset is cleared rather
+        // than left unchanged.
+        let reconciled = reconcile_disk_rate_limiter(
            &ConfigManager::global_config().machine.disk_rate_limit,


Considering using config.disk_rate_limit here, instead of global config.

github-actions · 2026-08-05T14:11:11Z

+        let cfg = &self.machine.disk_rate_limit;
+        if cfg.bandwidth_burst_bytes > 0 && cfg.bandwidth_bytes_per_sec == 0 {


[bug · medium]
Validation rejects burst/sustained combinations even when enabled is false, although both fresh-boot and snapshot reconciliation paths explicitly ignore all configured limits while disabled. This means dormant/pre-staged values can prevent application startup despite having no runtime effect. Return early when the feature is disabled (or otherwise document that disabled sections must still be internally valid).

Suggestion:

Suggested change

let cfg = &self.machine.disk_rate_limit;

if cfg.bandwidth_burst_bytes > 0 && cfg.bandwidth_bytes_per_sec == 0 {

let cfg = &self.machine.disk_rate_limit;

if !cfg.enabled {

return Ok(());

}

if cfg.bandwidth_burst_bytes > 0 && cfg.bandwidth_bytes_per_sec == 0 {

github-actions · 2026-08-05T14:11:11Z

+        if cfg.iops_burst > 0 && cfg.iops == 0 {
+            bail!(
+                "machine.disk_rate_limit: iops_burst is set but iops is 0; \
+                 a burst requires a nonzero sustained limit"
+            );
+        }
        Ok(())


[bug · medium]
The configuration fields accept values through u64::MAX, but Firecracker's token-bucket fields are i64. The consumer uses fallible conversions, so an enabled configuration above i64::MAX passes startup validation and then causes every affected sandbox creation/resume to fail. Validate each effective sustained/burst value against i64::MAX here so invalid operator input fails at config load time, consistently with this method's purpose.

Suggestion:

Suggested change

if cfg.iops_burst > 0 && cfg.iops == 0 {

bail!(

"machine.disk_rate_limit: iops_burst is set but iops is 0; \

a burst requires a nonzero sustained limit"

);

}

Ok(())

if cfg.iops_burst > 0 && cfg.iops == 0 {

bail!(

"machine.disk_rate_limit: iops_burst is set but iops is 0; \

a burst requires a nonzero sustained limit"

);

}

for (name, value) in [

("bandwidth_bytes_per_sec", cfg.bandwidth_bytes_per_sec),

("bandwidth_burst_bytes", cfg.bandwidth_burst_bytes),

("iops", cfg.iops),

("iops_burst", cfg.iops_burst),

] {

if value > i64::MAX as u64 {

bail!("machine.disk_rate_limit.{name} must be <= {}", i64::MAX);

}

}

Ok(())

github-actions · 2026-08-05T14:11:11Z

+/// Firecracker's `TokenBucket::size` is the number of tokens replenished every
+/// `refill_time`, not a per-second rate. Pinning the refill period to 1000 ms
+/// makes the configured `*_per_sec` values equal the sustained per-second rate.
+const RATE_LIMIT_REFILL_TIME_MS: i64 = 1000;


[bug · medium]
A 1-second refill interval also makes the regular bucket capacity equal to a full second of allowance. Firecracker permits consumption at unbounded speed while tokens are available, so an idle 100 MB/s drive can issue 100 MB immediately (in addition to one_time_burst) and then be throttled. This undermines the separately configured burst allowance and creates avoidable I/O latency spikes. Use a shorter interval and scale size to preserve the requested per-second rate (with explicit rounding/validation for low rates), rather than fixing the interval at one second.

We're fine with this trade-off and would rather not push the complexity onto users. No need to fix it.

github-actions · 2026-08-05T14:11:11Z

+    const UNLIMITED_BUCKET_SIZE: i64 = 1 << 40; // 1 TiB
+    const UNLIMITED_REFILL_TIME_MS: i64 = 1;


[bug · medium]
This does not actually clear the inherited limiter; it replaces it with a synthetic 1 PiB/s active limiter. Firecracker represents an explicit bucket disable in PATCH updates with a zero-sized bucket (whereas omission means unchanged). Using the disable representation provides exact semantics and avoids making every resume depend on acceptance and future behavior of unusually large limiter values.

This is correct, if you just want to reconcile the rate limiter, considering set its size or refill_time to zero. The current implementation of firecracker will disable rate limit when receive these arguments.

github-actions · 2026-08-05T14:11:11Z

+        let reconciled = reconcile_disk_rate_limiter(
+            &ConfigManager::global_config().machine.disk_rate_limit,
+        )?;


[bug · medium]
The fresh path applies the disk_rate_limit captured in FirecrackerSandboxConfig, but resume discards that provenance and reads the global config. A sandbox created through from_app_config_with_user_image can use a non-global AppConfig; after snapshotting it, this path silently changes its limit to the process-global value. Persist/carry the effective disk-rate config into FirecrackerSnapshotConfig (or otherwise pass the same effective config into resume) so fresh and restored instances are consistent.

- cfg: skip disk_rate_limit validation entirely when disabled - cfg: reject rate limit values exceeding i64::MAX with field-named errors - sandbox: use size-0 token bucket to disable a limiter dimension instead of an unlimited large-value bucket (exact Firecracker disable semantics) - config: carry disk_rate_limit in FirecrackerCommonConfig so fresh boot and snapshot resume read the same config and it persists across pause/resume, dropping the resume-path dependency on the global config

github-actions · 2026-08-06T06:57:28Z

+fn disabled_bucket() -> Box<firecracker_client::models::TokenBucket> {
+    // size = 0 is the disable signal; refill_time is irrelevant then but kept
+    // nonzero so the bucket is still structurally valid.
+    Box::new(firecracker_client::models::TokenBucket::new(1, 0))
+}


[bug · high]
Firecracker's PATCH disable representation requires both size and refill_time to be zero. A mixed bucket (size == 0, refill_time == 1) is not the disable sentinel and can be rejected as an invalid token bucket, causing every snapshot resume with an unset/disabled dimension to fail. Send { "size": 0, "refill_time": 0 } for BucketUpdate::Disabled.

Suggestion:

Suggested change

fn disabled_bucket() -> Box<firecracker_client::models::TokenBucket> {

// size = 0 is the disable signal; refill_time is irrelevant then but kept

// nonzero so the bucket is still structurally valid.

Box::new(firecracker_client::models::TokenBucket::new(1, 0))

}

fn disabled_bucket() -> Box<firecracker_client::models::TokenBucket> {

Box::new(firecracker_client::models::TokenBucket::new(0, 0))

}

huang-jl · 2026-08-06T08:11:45Z

Please fix the cargo fmt error, other codes look great to me.

- disabled_bucket now sends {size: 0, refill_time: 0}; a mixed bucket (size 0, refill_time 1) is not Firecracker's disable sentinel and can be rejected as invalid, failing snapshot resume for an unset dimension - fix rustfmt violation in instance.rs import grouping

github-actions · 2026-08-06T10:05:04Z

+    /// Enable per-sandbox disk I/O rate limiting via Firecracker's virtio-blk rate limiter.
+    #[config(default = false)]
+    pub enabled: bool,


[bug · medium]
This describes an aggregate per-sandbox disk limit, but the implementation attaches the limiter only to USER_ROOTFS_DRIVE_ID; the boot drive and all extra drives receive None, and extra drives can be writable. A workload can therefore bypass this advertised quota through a writable extra drive. Also, simply assigning the full limit independently to every drive would multiply the aggregate allowance. Either define/document this configuration explicitly as a user-rootfs-only limit (including the field/table naming), or implement aggregate enforcement across every writable sandbox drive with a quota-sharing/splitting policy.

Suggestion:

Suggested change

/// Enable per-sandbox disk I/O rate limiting via Firecracker's virtio-blk rate limiter.

#[config(default = false)]

pub enabled: bool,

/// Enable disk I/O rate limiting for the sandbox's user-rootfs drive via

/// Firecracker's virtio-blk rate limiter. Other attached drives are not limited.

#[config(default = false)]

pub enabled: bool,

github-actions · 2026-08-06T10:05:04Z

+    if cfg.bandwidth_burst_bytes > 0 {
+        bw.one_time_burst = Some(
+            i64::try_from(cfg.bandwidth_burst_bytes)
+                .context("disk bandwidth_burst_bytes exceeds Firecracker's i64 range")?,
+        );
+    }


[bug · medium]
one_time_burst is the bucket's initial size, not an additional allowance above size (see the vendored Firecracker model documentation). Thus the sample 100 MiB/s rate plus 10 MiB burst starts with only 10 MiB of tokens, rather than the documented 10 MiB above the normal 100 MiB bucket; it can even reduce the initial allowance below the no-burst behavior. The same issue applies to iops_burst. Either compute the initial size as size.checked_add(burst) (and validate overflow/range) to match the configuration docs, or rename/document these fields as absolute initial bucket sizes.

Suggestion:

Suggested change

if cfg.bandwidth_burst_bytes > 0 {

bw.one_time_burst = Some(

i64::try_from(cfg.bandwidth_burst_bytes)

.context("disk bandwidth_burst_bytes exceeds Firecracker's i64 range")?,

);

}

if cfg.bandwidth_burst_bytes > 0 {

let initial_size = cfg

.bandwidth_bytes_per_sec

.checked_add(cfg.bandwidth_burst_bytes)

.context("disk bandwidth rate plus burst overflows u64")?;

bw.one_time_burst = Some(

i64::try_from(initial_size)

.context("disk bandwidth rate plus burst exceeds Firecracker's i64 range")?,

);

}

I've checked the Firecracker source code and confirm this AI-generated comment is correct: one_time_burst is a one-off token allowance granted at startup. Once consumed, it is not replenished. It's designed to absorb the temporary I/O spikes that occur during boot.

That said, this is fine since it's user-configured. My only concern is that the PR description may be a bit misleading.

The PR looks good to me, and we should be able to merge it soon.

Document bandwidth/IOPS burst fields as Firecracker one_time_burst: a separate allowance granted once at VM start, consumed before the sustained bucket and not replenished, so it absorbs the initial I/O spike rather than raising the steady-state rate.

github-actions · 2026-08-06T12:24:24Z

+# bandwidth_burst_bytes = 10485760      # 10 MB one-time burst at VM start (not replenished)
+# iops = 3000
+# iops_burst = 500                       # one-time IOPS burst at VM start (not replenished)


[documentation · low]
“At VM start” is misleading for snapshot restores. The resume path PATCHes a newly constructed rate limiter before every resume(), including one_time_burst, so Firecracker can grant this allowance again on each restore/resume rather than only at the original VM boot. Please document that the burst is one-time per limiter application (fresh boot or snapshot restore), or avoid sending it during resume if the intended contract is truly once per VM lifecycle.

Suggestion:

Suggested change

# bandwidth_burst_bytes = 10485760 # 10 MB one-time burst at VM start (not replenished)

# iops = 3000

# iops_burst = 500 # one-time IOPS burst at VM start (not replenished)

# bandwidth_burst_bytes = 10485760 # 10 MiB one-time burst per limiter application (fresh boot or snapshot restore)

# iops = 3000

# iops_burst = 500 # one-time IOPS burst per limiter application

github-actions · 2026-08-06T12:24:24Z

+        // Both buckets are always overwritten (configured or unlimited) so an
+        // inherited dimension the current config leaves unset is cleared rather
+        // than left unchanged.
+        let reconciled = reconcile_disk_rate_limiter(&config.common.disk_rate_limit)?;


[bug · medium]
This does not actually reconcile against the node's current configuration as the preceding comment claims. On committed-snapshot launches, config.common comes from FirecrackerSnapshotConfig::from_runnable_snapshot, and snapshot_config_for_launch does not replace disk_rate_limit; therefore changing machine.disk_rate_limit after a snapshot was created has no effect, and a snapshot can retain an obsolete (or disabled) policy indefinitely. Use the current app config here, or explicitly overwrite this field while constructing a snapshot launch config. If persisted policy is intentional instead, the “node's current config” behavior/documentation should be corrected.

Suggestion:

Suggested change

let reconciled = reconcile_disk_rate_limiter(&config.common.disk_rate_limit)?;

let reconciled = reconcile_disk_rate_limiter(

&ConfigManager::global_config().machine.disk_rate_limit,

)?;

github-actions · 2026-08-06T12:24:24Z

+        assert_eq!(bw.size, 0);
+        assert_eq!(ops.size, 0);


[test · low]
The implementation relies on the exact (size = 0, refill_time = 0) Firecracker disable sentinel, but this test checks only size. A regression producing a mixed bucket would still pass and could break every snapshot resume. Assert refill_time == 0 for both buckets as well; the bandwidth-only and IOPS-only reconciliation tests should likewise validate the disabled bucket's refill time.

Suggestion:

Suggested change

assert_eq!(bw.size, 0);

assert_eq!(ops.size, 0);

assert_eq!(bw.size, 0);

assert_eq!(bw.refill_time, 0);

assert_eq!(ops.size, 0);

assert_eq!(ops.refill_time, 0);

github-actions Bot reviewed Jul 28, 2026

View reviewed changes

yingdi-shan requested a review from huang-jl July 29, 2026 02:01

rogeroger-yu force-pushed the feat/disk-rate-limit-firecracker branch from 35f51d9 to ef01c5c Compare July 29, 2026 02:33

github-actions Bot reviewed Jul 29, 2026

View reviewed changes

huang-jl reviewed Jul 29, 2026

View reviewed changes

github-actions Bot reviewed Aug 4, 2026

View reviewed changes

github-actions Bot reviewed Aug 5, 2026

View reviewed changes

huang-jl reviewed Aug 5, 2026

View reviewed changes

github-actions Bot reviewed Aug 5, 2026

View reviewed changes

github-actions Bot reviewed Aug 6, 2026

View reviewed changes

huang-jl approved these changes Aug 6, 2026

View reviewed changes

yingdi-shan approved these changes Aug 7, 2026

View reviewed changes

yingdi-shan merged commit 0369df1 into kvcache-ai:main Aug 7, 2026
7 checks passed

		cfg.refill_time_ms as i64,
		cfg.bandwidth_bytes_per_sec as i64,

-        let mut bw = firecracker_client::models::TokenBucket::new(
-            cfg.refill_time_ms as i64,
-            cfg.bandwidth_bytes_per_sec as i64,
-        );
+        let refill_time = i64::try_from(cfg.refill_time_ms)
+            .context("disk rate-limit refill_time_ms exceeds Firecracker's i64 range")?;
+        let bandwidth_size = cfg
+            .bandwidth_bytes_per_sec
+            .checked_mul(cfg.refill_time_ms)
+            .and_then(|value| value.checked_div(1000))
+            .and_then(|value| i64::try_from(value).ok())
+            .context("disk bandwidth rate limit is outside Firecracker's supported range")?;
+        let mut bw = firecracker_client::models::TokenBucket::new(refill_time, bandwidth_size);

		self.fc_instance.start().await?;
		self.apply_disk_rate_limiter(false).await?;

		let cfg = &ConfigManager::global_config().machine.disk_rate_limit;
		let rl = match build_disk_rate_limiter(cfg)? {

		self.apply_disk_rate_limiter(&config.disk_rate_limit, false)
		.await?;

		let cfg = &self.machine.disk_rate_limit;
		if cfg.bandwidth_burst_bytes > 0 && cfg.bandwidth_bytes_per_sec == 0 {

-        if cfg.iops_burst > 0 && cfg.iops == 0 {
-            bail!(
-                "machine.disk_rate_limit: iops_burst is set but iops is 0; \
-                 a burst requires a nonzero sustained limit"
-            );
-        }
-        Ok(())
+        if cfg.iops_burst > 0 && cfg.iops == 0 {
+            bail!(
+                "machine.disk_rate_limit: iops_burst is set but iops is 0; \
+                 a burst requires a nonzero sustained limit"
+            );
+        }
+        for (name, value) in [
+            ("bandwidth_bytes_per_sec", cfg.bandwidth_bytes_per_sec),
+            ("bandwidth_burst_bytes", cfg.bandwidth_burst_bytes),
+            ("iops", cfg.iops),
+            ("iops_burst", cfg.iops_burst),
+        ] {
+            if value > i64::MAX as u64 {
+                bail!("machine.disk_rate_limit.{name} must be <= {}", i64::MAX);
+            }
+        }
+        Ok(())

		const UNLIMITED_BUCKET_SIZE: i64 = 1 << 40; // 1 TiB
		const UNLIMITED_REFILL_TIME_MS: i64 = 1;

Uh oh!

Conversation

rogeroger-yu commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Notes on burst semantics

Changes

Test plan

Uh oh!

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

huang-jl Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

rogeroger-yu commented Aug 4, 2026

Follow-up: fixed resume reconciliation + e2e validation

e2e setup

Results

Root cause

Fix

Uh oh!

github-actions Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

huang-jl Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

huang-jl Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

huang-jl Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

rogeroger-yu commented Jul 28, 2026 •

edited

Loading

github-actions Bot commented Jul 28, 2026 •

edited

Loading

huang-jl Aug 4, 2026 •

edited

Loading